Skip to content

Fix tenant ID validation error handling in sam local start-api#8453

Merged
roger-zhangg merged 4 commits into
developfrom
patch
Nov 21, 2025
Merged

Fix tenant ID validation error handling in sam local start-api#8453
roger-zhangg merged 4 commits into
developfrom
patch

Conversation

@licjun

@licjun licjun commented Nov 20, 2025

Copy link
Copy Markdown
Contributor

Add proper exception handling for TenantIdValidationError in API Gateway
service to return HTTP 400 Bad Request with the actual validation message
instead of generic "Internal server error".

Before this change, if user do sam local start-api and invoke a multi-tenant function without tenant-id header provided, user will see an internal server error, with this change user will see {"message":"The invoked function is enabled with tenancy configuration. Add a valid tenant ID in your request and try again.}

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

… without tenant-id header when using sam local start-api
@licjun
licjun requested a review from a team as a code owner November 20, 2025 22:32
@github-actions github-actions Bot added area/local/start-api sam local start-api command area/local/invoke sam local invoke command area/local/start-invoke labels Nov 20, 2025
Comment thread samcli/local/apigw/local_apigw_service.py Dismissed
@roger-zhangg

Copy link
Copy Markdown
Member

let's add a test to make sure this error is throw as expected

@roger-zhangg roger-zhangg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this test actually doing anything? Is it like you are throwing an TenantIdValidationError in the test and assert the same error is thrown? Ideally we should just invoke the function with the condition (tenancy function without tenant ID, or non-tenant function with tenant ID) and assert the error. You should not use invoke_mock.side_effect directly. Or it's like the test is testing itself

@licjun

licjun commented Nov 21, 2025

Copy link
Copy Markdown
Contributor Author

Is this test actually doing anything? Is it like you are throwing an TenantIdValidationError in the test and assert the same error is thrown? Ideally we should just invoke the function with the condition (tenancy function without tenant ID, or non-tenant function with tenant ID) and assert the error. You should not use invoke_mock.side_effect directly. Or it's like the test is testing itself

This test is specifically testing error handling, not validation logic.

The actual tenant-id validation logic is already tested in tests/unit/commands/local/lib/test_local_lambda.py in the TestLocalLambda_tenant_id_validation class, which includes:
• test_multi_tenant_function_without_tenant_id_raises_error
• test_non_multi_tenant_function_with_tenant_id_raises_error
• test_multi_tenant_function_with_tenant_id_succeeds
(https://github.com/aws/aws-sam-cli/blob/develop/tests/unit/commands/local/lib/test_local_lambda.py#L1006)
Those tests properly invoke functions with real configurations and let the actual validation logic run.

For the API Gateway service test, the responsibility is different - it needs to verify that when TenantIdValidationError is raised (from the lambda runner), the API Gateway service correctly:

  1. Catches the exception
  2. Converts it to an HTTP 400 response
  3. Returns the proper error message in JSON format

@roger-zhangg roger-zhangg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's add test back after release

Comment on lines +743 to +744
response_data = jsonify({"message": str(e)})
endpoint_service_error = make_response(response_data, 400) # HTTP 400 Bad Request

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better to be consistent with the other Errors and add a new method inside the ServiceErrorResponses class instead of calling make_response right here.

But it's just a code refactor that's not critical.

@roger-zhangg
roger-zhangg added this pull request to the merge queue Nov 21, 2025
Merged via the queue into develop with commit 2fd626b Nov 21, 2025
41 checks passed
@licjun
licjun deleted the patch branch December 1, 2025 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants